Skip to content

FPM Feature "ping.dontlog" : See Issue #8174 #8184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

Julien75013
Copy link

See Issue : #8174

@bukka
Copy link
Member

bukka commented Mar 9, 2022

I would prefer some sort of basic filtering to the access log instead (could be just path for now but something that could be possible to potentially extend in the future). This seems a bit hacky to me. Also any such change will require a test.

@Julien75013
Copy link
Author

@bukka Sure, however it's a ping feature, not a filtering feature, that need more discussion about it.

This little change, makes a big saving data on the disk ; referring to the initial issue. #8184 🙂🍀

@Julien75013 Julien75013 changed the title Feature "ping.dontlog" : See Issue #8174 FPM Feature "ping.dontlog" : See Issue #8174 Mar 9, 2022
@bukka
Copy link
Member

bukka commented Mar 9, 2022

I understand that it's useful but technically it's an access log feature because that's what you want to reduce. It's basically just filtering of the ping path so it would be nicer to have this configured as part of access log in a more generic way..

@Julien75013
Copy link
Author

Julien75013 commented Mar 9, 2022

We both agree on the same facts. Not sufficient for the future, a bit hacky, but useful as soon as it could be implemented. It would be better than nothing for now, and could be replaced later, by a better filtering system. I ignore your policies about dilemmas, between having an immediate improvement (~30Mo per FPM server with "/ping" enabled), and having a more generic code.

Anyway it doesn't prevent to replace this filter, by another one better, later. If anyone can start a generic template, i could be able to bring my help, but i don't have any idea for now, about your generic coding style. I usually use bash, php, haproxy, apache and a lot of other softwares, I do not a lot of C, but can follow anyone in such direction after a template would have begun.

@maaarghk
Copy link
Contributor

i was coincidentally just looking for this, but would prefer to have it be a list of filtered request URIs since I have a more complex /health_check.php being called.

I could make a PR with a new parameter like:

[global]
access_log.exclude_request_uri[] = /ping
access_log.exclude_request_uri[] = /health_check.php

array format follows parse_ini_file from core, although fpm_conf_set_array does not support empty keys yet.

would you be inclined to accept that @bukka ?

@maaarghk
Copy link
Contributor

well if we're talking generic you might get some people who want to exclude on multiple keys:

[global]
access_log.exclude_request_uri[] = /ping
access_log.exclude_request_uri[] = /health_check.php
access_log.exclude_remote_addr[] = 127.0.0.1
access_log.exclude_x_forwarded_for[] = 172.17

I wouldn't really know where to begin benchmarking whether using a regex would be too resource intensive, i was picturing using strstr to do a "begins with" check.

@maaarghk
Copy link
Contributor

By the way, does it make sense to only exclude when the return code is 200? I feel like I'd want to know %p, %C, %M for failed requests.

@Julien75013
Copy link
Author

Julien75013 commented Mar 14, 2022

The initial purpose is to reduce the access log file size (with a profit in I/O i guess), due to the health checks, what is an FPM feature. When the "ping path" is reached under FPM, it does not permit to change the configuration on the fly.

This is why, after some reflection, I maintain my first idea: in this precise case, an option must also be specific.

Then, for all the others filters, it is more conveniant and generic to use the Userland implementation, it should be faster without dealing a lot of rules on each request, what do you think ? Except if I miss something, tell me know.

[global]
log.access_policy = On

<?php
// don't log the request, with your conditions
ini_set('log.access_policy', '0');
?>

@bukka
Copy link
Member

bukka commented Mar 16, 2022

I think single entry filter should be fine. It could really just be some list of ignore paths because that's what's really needed. The main use case is to not just cover ping but also have option to cover status path which can add up to the logs as well. We shouldn't really go for full regexp as that's quite heavy - static list should be a good start and maybe some prefix wildcard would be quite easy as well.

@maaarghk
Copy link
Contributor

maaarghk commented Mar 17, 2022

ok, I started working on a PR to accept an array of ignore paths.

My thinking behind the remote addr example above is that a health check script would become a good place to evade detection when taking advantage of an exploit. Since we won't implement complex filters right now, I think that the "suppress log" flag should be ignored if there is any request body, query params, or request method other than GET or HEAD, the path filter should be exact match only, the response code should be 200.

I actually do concur with the point of @Julien75013 that a complex filter could be implemented by applications, where conf file could be left to do the straightforward cases. But an ini value won't be trivial to implement because the fpm.conf values aren't accessible to ini_set. Instead I think we could expose a function called fpm_suppress_access_log to set the suppress log flag. It could mean that frameworks or applications ship usage of this as part of their provided health check scripts. Any sysadmin that disagrees with it could stick in php_admin_value[disable_functions]=fpm_suppress_access_log.

All that seem sensible?

@maaarghk
Copy link
Contributor

I started over in #8214 if you want to take a look.

@bukka
Copy link
Member

bukka commented Apr 23, 2022

I'm going to close this in favor of #8214 .

@bukka bukka closed this Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants